-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(menu-full-screen, modal): ensure the call to action element is focused on close #6986
base: master
Are you sure you want to change the base?
Conversation
b0ac69d
to
d3ad27a
Compare
7c428ef
to
cd15a0e
Compare
720eeef
to
8c7988c
Compare
@@ -54,6 +54,93 @@ export const DialogFullScreenComponent = ({ | |||
); | |||
}; | |||
|
|||
export const DefaultStory = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: do we need these test cases added? It looks like we could use some existing ones an pass an open
prop which initialises the state
@@ -80,17 +95,31 @@ const useModalManager = ({ | |||
if (modalRegistered.current) { | |||
ModalManager.removeModal(ref, triggerRefocusOnClose); | |||
|
|||
if (lastFocusedElement.current) { | |||
setTimeout(() => { | |||
lastFocusedElement.current?.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastFocusedElement.current?.focus(); | |
lastFocusedElement.current.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be necessary since the call is made within an asynchronous function. Although it’s unlikely the ref will change during the very short timeout, TypeScript may still consider it possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unfortunately I'm still getting a possibly null warning, to avoid it I'd have to do have an if statement asserting lastFocusedElement.current
again within the setTimeout
to avoid it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, it won't ever be null if it passes the guard as the only point that happens is in the callback the timeout is executing but I get why TS can't guarantee it when it computes the types
modalRegistered.current = false; | ||
} | ||
}, | ||
[triggerRefocusOnClose] | ||
); | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I think we can initialise this earlier like so but if it doesn't work you could use a layout effect
const modalOpenOnRender = useRef(open); // init it straightaway
...
useEffect(() => {
// block and reset etc
if (modalOpenOnRender.current) {
modalOpenOnRender.current = false;
return;
}
if (
!lastFocusedElement.current &&
focusCallToActionElement &&
open
) {
lastFocusedElement.current = focusCallToActionElement;
}
}, [open, focusCallToActionElement]);
useLayoutEffect(() => {
modalOpenOnRender.current = true;
}, []);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think due to the modalOpenOnRender
being made redundant now, we can avoid the useLayoutEffect
and reduce the useEffect
too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this @tomdavies73 👍🏼
@@ -54,6 +54,58 @@ export const DialogFullScreenComponent = ({ | |||
); | |||
}; | |||
|
|||
export const DefaultStory = ({ open = false }: { open?: boolean }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: were you not able to tweak DialogComponent
to achieve the same functionality as this story? It seems like you could just add a button to it etc
); | ||
}; | ||
|
||
export const DefaultNestedStory = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: isn't this pretty much the same as the NestedStory
below?
@@ -210,8 +210,8 @@ export const DialogComponentFocusableSelectors = ( | |||
); | |||
}; | |||
|
|||
export const DefaultStory = () => { | |||
const [isOpen, setIsOpen] = useState(defaultOpenState); | |||
export const DefaultStory = ({ open }: { open?: boolean }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking etc
export const DefaultStory = ({ open }: { open?: boolean }) => { | |
export const DefaultStory = ({ open = defaultOpenState }: { open?: boolean }) => { | |
const [isOpen, setIsOpen] = useState(open); |
@@ -304,6 +304,92 @@ export const MenuComponentFullScreen = ( | |||
); | |||
}; | |||
|
|||
export const MenuComponentFullScreenOpen = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: apologies if I missed this on the first review, it just seems to me that this way more complex than it needs to be:
- do we need to support all the menuTypes? To me it's not important here we could just have two of the default
- do we need to concern ourselves with the breakpoints here? To me we could just render it as it's also superfluous to what we're testing etc
- I'd also reduce the menu-items down to a couple etc but that's less of an issue
@@ -29,6 +29,63 @@ export const Default = (args: Partial<SidebarProps>) => { | |||
); | |||
}; | |||
|
|||
export const DefaultClosed = (args: Partial<SidebarProps>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: I think this one could be replaced by adding an open
prop to SidebarComponent
with a default of true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this has been done in the most recent commit
await expect(button).toBeFocused(); | ||
}); | ||
|
||
test("when nested Sidebar's are open/closed their respective call to action elements should be focused correctly", async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: open
-> opened
@@ -80,17 +95,31 @@ const useModalManager = ({ | |||
if (modalRegistered.current) { | |||
ModalManager.removeModal(ref, triggerRefocusOnClose); | |||
|
|||
if (lastFocusedElement.current) { | |||
setTimeout(() => { | |||
lastFocusedElement.current?.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, it won't ever be null if it passes the guard as the only point that happens is in the callback the timeout is executing but I get why TS can't guarantee it when it computes the types
src/components/dialog/dialog.pw.tsx
Outdated
await expect(button).toBeFocused(); | ||
}); | ||
|
||
test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({ | |
test("when nested Dialog's are opened/closed their respective call to action elements should be focused correctly", async ({ |
await expect(button).toBeFocused(); | ||
}); | ||
|
||
test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test("when nested Dialog's are open/closed their respective call to action elements should be focused correctly", async ({ | |
test("when nested Dialog's are opened/closed their respective call to action elements should be focused correctly", async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the new changes @tomdavies73 👍🏼 Just one small question from me in addition to @edleeks87's comments
…cused on close Ensures the element which opens `Modal` or a fullscreen `Menu` is focused when the respective component closes. fix #6870
I did an @desk demo with @tempertemper and this also passes accessibility review. |
fix #6870
Proposed behaviour
Ensures focus is moved back onto the active element before a modal/fullscreen menu is opened.
Current behaviour
Currently, once a modal is closed, focus is typically returned to the document, which can be disorientating for users.
Checklist
d.ts
file added or updated if requiredQA
Additional context
Testing instructions